Validate single row count for Row.Scan #460
Conversation
stevenh
force-pushed
the
get-multi-rows
branch
2 times, most recently
from
f8641fc to
3def8f8
Compare
Pull Request Test Coverage Report for Build 53
💛 - Coveralls |
Pull Request Test Coverage Report for Build 55
💛 - Coveralls |
There was a problem hiding this comment.
Looks good, though this does change behaviour of a core method (to more correct behaviour in my opinion), which has the potential to cause a lot of third-party code to break.
Not sure if it would be better to make this a toggle-able behaviour (like enabling a 'strict' mode), or adding a new method like One() which acts like Get() but performs this stricter validation on the results set, so that it doesn't break existing applications that are (incorrectly) relying on this behaviour.
sqlx.go
Outdated
| @@ -15,6 +15,12 @@ import ( | |||
| "github.com/jmoiron/sqlx/reflectx" | |||
| ) | |||
|
|
|||
| // ErrMultiRows is returned by functions which are expected to work with result sets | |||
| // that only contain a single row but multiple rows where returned. | |||
sqlx.go
Outdated
| @@ -15,6 +15,12 @@ import ( | |||
| "github.com/jmoiron/sqlx/reflectx" | |||
| ) | |||
|
|
|||
| // ErrMultiRows is returned by functions which are expected to work with result sets | |||
| // that only contain a single row but multiple rows where returned. | |||
|
How about instead of making this optional you create a 1.2.0 release and merge this after the release? It might still break some users code but it also reminds them to stick to a released version. Otherwise you end up never merging features that fix broken behavior. |
|
I think its important this isn't gated and does cause failures as the users code is currently broken in an unpredictable way and they just don't know it. With this fix in our code base every single case was a legitimate bug. |
Validate that a single row only is returned by queries used for Row.Scan. This avoids unexpected results when the query has an issue such as a missing join criteria or limit in conjunction with functions which expect only on row returned e.g. Get(...). Also: * Fixed missing \n's for test output of ConnectAll.
stevenh
force-pushed
the
get-multi-rows
branch
from
3def8f8 to
7fe0fee
Compare
|
@glaslos not a fan of that process, as it means no one will notice and fix till the next release. I would always expect those tracking master to be aware there may be noticeable changes, such as this one, and to avoid those one should be tracking a release branch. |
|
@stevenh I very much agree with your sentiment. Something very similar happened to uuid: satori/go.uuid#66 which eventually resulted in the community forking the project, leaving behind a lot of broken code and outrage. |
|
Having read that thread it was an API breaking change where this is more a correction of missing error, which I don't think anyone would object to knowing about. There's also a very easy fix if you SQL is broken and you don't want to fix it properly add As a way of an update this fix has found more subtle bugs in our codebase since being deployed so continuing to have a positive impact. |
|
I know this is really old, but just checking to see if we can get this one in? |
|
A friendly bump to see if we can get this in @jmoiron ? |
|
@ardan-bkennedy this looks like a good change, but probably in a new major release since it breaks behavior. Wdyt? |
dlsniper
added
the
requires attention
Validate that a single row only is returned by queries used for Row.Scan.
This avoids unexpected results when the query has an issue such as a missing join criteria or limit in conjunction with functions which expect only on row returned e.g.
Get(...).Also:
ConnectAll.